Add support for Redis JSON#3390
Conversation
Signed-off-by: Yordan Tsintsov <yordan.tsintsov@redis.com>
Signed-off-by: Yordan Tsintsov <yordan.tsintsov@redis.com>
Signed-off-by: Yordan Tsintsov <yordan.tsintsov@redis.com>
Signed-off-by: Yordan Tsintsov <yordan.tsintsov@redis.com>
mp911de
left a comment
There was a problem hiding this comment.
Here's an initial round of review feedback.
| super.afterPropertiesSet(); | ||
|
|
||
| if (keySerializer == null) { | ||
| keySerializer = (RedisSerializer<K>) new JdkSerializationRedisSerializer(classLoader != null ? classLoader : this.getClass().getClassLoader()); |
There was a problem hiding this comment.
Let's default to String serializers. Zooming out, a constructor RedisJsonTemplate(RedisConnectionFactory) with String and a default JSON serializer(Jackson 2/Jackson 3) makes sense. Also, a constructorRedisJsonTemplate(RedisConnectionFactory, RedisSerializer, RedisJsonSerializer)` as alternative to provide serializers.
Moving off mutable state is a good thing. We can still keep transaction support but even for transactions I am not sure we should do this.
There was a problem hiding this comment.
After discussing with @mp911de and @Dgramada, we agreed to mark transaction support as out of scope for this feature. @Dgramada will remove the related code.
We also agreed that RedisJsonTemplate should default to a string serializer for keys and RedisJsonSerializer for values (the actual JSON). @Dgramada explored the proposed constructor approach, but it led to this line:
this.keySerializer = (RedisSerializer<K>) RedisSerializer.string();
which would break if K is anything other than String. Following @mp911de's suggestion, we'll explore a static factory method next and see whether it surfaces any other issues.
Signed-off-by: Yordan Tsintsov <yordan.tsintsov@redis.com>
Signed-off-by: Yordan Tsintsov <yordan.tsintsov@redis.com>
Signed-off-by: Yordan Tsintsov <yordan.tsintsov@redis.com>
This is a follow up pull request to #3327 that handles the new JSON API for Spring Data Redis.
It builds upon the command layer from the referenced PR and adds API for the template layer.
Notes:
BoundJsonOperationsis not included yet. Once a version ofJsonOperationsis approved, the bound operations can be included in this branch.RedisJsonTemplateinitially felt like a good idea. It creates a separation of concerns, which is nice because the new JSON API does not pair well with a specificVtype unlike the other data types inRedisTemplate. However,RedisJsonTemplatefeels a little off and maybe a more elegant solution could be derived. It feels more like a configuration class. Maybe we could rename it toRedisJsonConfig. This would also promote developers to injectJsonOperationsdirectly.RedisJsonSerializerdoes not extendRedisSerializer, due to it bringing a lot of functionality that doesn't add any value to the JSON serialization. Furthermore, keeping them separate gives separation of concerns. Also Jedis and Lettuce do not provide byte[] APIs for the JSON commands in Redis, thus makingRedisSerializerincompatible.